🤖 I'm worried this mutable flag races across concurrent queries that share a FileFormat instance via a temp view. If query A (uses read_blob()) sets the flag to false during its optimization, and query B (no read_blob()) runs concurrently and calls restoreBlobDescriptorMode() before A's tasks are serialized, A's executors will deserialize the FileFormat with flag=true — the executor-side readBaseFile (line 565) reads isBlobDescriptorMode and would strip the data column, causing read_blob() to silently return null. Same hazard in the opposite direction for B. Async query submission (e.g. Futures, Airflow, multi-cell notebooks) makes this realistic. Have you considered passing the per-query mode through the LogicalRelation's options map / Hadoop conf at rule time instead of mutating shared state? @nsivabalan @yihua thoughts on whether this concurrency surface is acceptable here?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 For MOR tables with log files, the log-file branch (line 123-126) reads with the full requiredSchema, so log records keep their populated data field, while base-file records under DESCRIPTOR get data=null. After merge the user sees a mix: records updated via log have bytes, records still in base have null. Is this the intended semantics, or should DESCRIPTOR also null the data on log-file rows for consistency?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 syncDescriptorMode is a side effect inside an optimizer rule's apply. Spark's optimizer batches typically run rules to a fixed point, so this can fire multiple times per query. It's idempotent within a single query (good), but the side effect on a shared FileFormat ordering between concurrent queries' optimizer passes is what creates the race I flagged in HoodieFileGroupReaderBasedFileFormat. Have you considered mapChildren / structural rewrites to encode the mode in the plan (e.g. as a hint or option) rather than mutating the FileFormat?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: containsReadBlobAnywhere is dangerously close to the existing containsReadBlobInAnyExpression just above it, but does something meaningfully different — it walks the entire plan tree rather than checking only the current node's expressions. A name like planTreeContainsReadBlob or treeContainsReadBlob would make the scope distinction immediately obvious to the next person who touches this rule.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: syncDescriptorMode reads like a concurrency-sync operation rather than a policy-apply step. Something like applyDescriptorModePolicy or updateDescriptorModeForPlan would make it clearer that this is deciding whether to downgrade DESCRIPTOR→CONTENT based on query content, not synchronizing threads.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 Heads-up: @Ignore is from JUnit 4 (org.junit.Ignore), but the surrounding test class uses JUnit 5 (@Test from org.junit.jupiter.api). The Jupiter engine doesn't honor JUnit 4's @Ignore, so this test will actually execute rather than be skipped — the rest of the file uses @Disabled from org.junit.jupiter.api.Disabled for that purpose. Given the TODO to re-enable comment, was the intent to skip this? If so, switching to @Disabled("reason") would do it.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: could you rename blobDescriptorMode to isBlobDescriptorMode? It's a boolean and the constructor parameter it's passed into (isBlobDescriptorMode) already uses the is prefix — the local val not following suit is a small but jarring inconsistency.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: the body of this wrapWithBlobNullPadding (create projection → create mapper → wrap iterator) is almost identical to SparkFileFormatInternalRowReaderContext.wrapWithBlobNullPadding at line 421 of that file — only the iterator type differs. Have you considered consolidating the shared setup into VectorConversionUtils or a small shared utility so the two call sites can't silently diverge if buildBlobNullPadRowMapper's signature changes?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

We should flip the default to DESCRIPTOR and let all table services to still use CONTENT mode.

My understanding is that this internal config intends to differentiate the blob column reading of read_blob(col1) vs col2 in SELECT read_blob(col1), col2 FROM table so that col1 is read out with CONTENT and col2 is read out with DESCRIPTOR. However, Lance reader can only be configured with CONTENT or DESCRIPTOR mode for all blob columns. Should we revisit the expected behavior of hoodie.read.blob.inline.mode?

To simplify the experience, it makes sense for user to only have read_blob(col) or col in the same query where col is a blob column. If a mix of both exist, all blob columns are read out with content.

Thus, all blob columns are read in DESCRIPTOR mode if no read_blob(col) is used. All blob columns are read in CONTENT mode if any read_blob(col) exists in the query.

Should we consider thread-safe?

Is there a case that read_blob() silently returns null to the user instead of throwing where it should not?

new GenericInternalRow(3) allocates per blob column per row on a hot path. For a 1M-row scan with 2 blob columns that's 2M short-lived heap allocations.

Also, the hardcoded 3 is fragile.

Matching on a.name is fragile if users alias blob columns (SELECT read_blob(payload) AS p ...) or if attribute names are qualified differently between the plan and the file schema. The reader side compares against parquetReadStructType.fields(idx).name (literal Parquet column name) in SparkFileFormatInternalRowReaderContext.scala:123 and HoodieFileGroupReaderBasedFileFormat.scala:510. Any divergence silently misses the override and read_blob() returns null.

HadoopFsRelation's constructor signature has varied across Spark 3.3/3.4/3.5 (e.g. userSpecifiedSchema was added in 3.4). If Hudi still supports the older Spark profile this will fail to compile cross-version. Verify against pom.xml's Spark profile matrix, or use rel.copy(options = newOptions) if Scala's case-class copy is available on this Spark version.